Align DataFrame.reindex dtype and validation behavior with pandas - #22914
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdates reindexing behavior, numeric dtype matching, datetime index frequency preservation, empty-sequence inserts, categorical count filling, and related pandas testing-plugin expected-failure mappings. ChangesPandas-compat reindex and indexing fixes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/cudf/cudf/core/indexed_frame.py`:
- Around line 4142-4147: The condition checking `col.dtype.kind in "iu"` matches
both numpy integer dtypes and pandas nullable integer dtypes like `Int64`, but
the upcast to float64 should only apply to numpy dtypes that cannot represent
nulls natively. Add an additional check to exclude pandas extension dtypes
(nullable integer types) before performing the astype conversion to float64,
ensuring that only numpy integer columns are upcasted while pandas nullable
integer columns are left unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 533d591e-aae2-4339-8fba-1becf7b7514d
📒 Files selected for processing (3)
python/cudf/cudf/core/dataframe.pypython/cudf/cudf/core/indexed_frame.pypython/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
Includes the predicate in the physical plan explain output for `ConditionalJoin`, `Filter`, and `Scan`. Purely an improvement; helps make studying plan easier. Authors: - Matthew Murray (https://github.com/Matt711) Approvers: - Matthew Roeschke (https://github.com/mroeschke) URL: NVIDIA#22984
…#18171) Part of NVIDIA#16481 Since theres two consumers of this API now (pandas and polars), I am wondering if we might take another look at adding libcudf APIs here. xref NVIDIA#16802 and cc @bdice @mroeschke @galipremsagar . WIP Authors: - https://github.com/brandon-b-miller - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Matthew Murray (https://github.com/Matt711) - Matthew Roeschke (https://github.com/mroeschke) URL: NVIDIA#18171
We run upstream polars tests with cudf-polars using two of or engines: in-memory and SPMD. The recent CI failures running upstream polars tests have all been in the SPMD engine, while the in-memory engine has been passing fine. This change reduces the pytest verbosity for the in-memory run (making the logs easier to scan), and increases it for the SPMD run (hopefully helping with identifying the flaky tests). Authors: - Tom Augspurger (https://github.com/TomAugspurger) Approvers: - Gil Forsyth (https://github.com/gforsyth) - Matthew Murray (https://github.com/Matt711) URL: NVIDIA#22980
…aN (NVIDIA#22818) Closes NVIDIA#22817. A `float`/`double` column containing a NaN mixed with non-NaN values was getting column-chunk min/max statistics computed from the non-NaN values only. The min/max reduce in `cpp/src/io/statistics/typed_statistics_chunk.cuh` uses `cuda::std::min/max` over `extrema_type::convert(val)` (which returns the value unchanged), and since `NaN < x` / `NaN > x` are both false a NaN never wins the reduce and is silently dropped — so the chunk ends up with a defined min/max that ignores the NaN. Per the Parquet convention adopted in parquet-mr (PARQUET-1246), a floating-point column that contains any NaN must omit min/max; otherwise a reader doing predicate pushdown (e.g. `col = NaN`, or a range predicate) uses the bogus bounds to skip the row group and silently drops valid rows. An all-NaN column was safe only incidentally — the merge's `has_minmax = (minimum_value <= maximum_value)` check flips it off because the reduce leaves the accumulator at inverted identity — but the mixed NaN + non-NaN case passes that check. Found via NVIDIA/cudf-spark#15004 (a GPU-written file read back by CPU Spark with a `= NaN` predicate-pushdown filter returned 0 rows instead of the matching NaN row). Affects any float/double leaf, top-level or nested in list/struct/map at any depth. ### Fix Track whether a NaN was seen during the float/double chunk reduce (`has_nan`), propagate it through `block_reduce` and the chunk merge, and force `has_minmax = false` for Parquet when it is set. ORC is unchanged: the guard is `if constexpr (IO == PARQUET)`, and the flag is only carried (never acted on) for ORC. Added `ParquetWriterTest.FloatingPointWithNaNStatsOmitted` (float / double / all-NaN must omit min/max; a no-NaN control still writes them). `ParquetWriterTest` (54 tests) and the `*Stats*` / `*ColumnIndex*` suites (45 tests) pass locally. Authors: - Allen Xu (https://github.com/wjxiz1992) Approvers: - Muhammad Haseeb (https://github.com/mhaseeb123) URL: NVIDIA#22818
…22945) Table: `--explain-partition-plan` ``` Partition Plan Summary +----+----------+--------+--------------+-------+------------------------+-----------+------------+ | Q | Table | Flavor | Factor | Files | Projected (bytes/file) | Size/task | Partitions | +----+----------+--------+--------------+-------+------------------------+-----------+------------+ | 1 | lineitem | SPLIT | 3 tasks/file | 60 | 4.35 GB | 1.45 GB | 180 | | 5 | region | FUSED | 1 file/task | 1 | 114 B | 114 B | 1 | | | nation | FUSED | 1 file/task | 1 | 329 B | 329 B | 1 | | | customer | FUSED | 1 file/task | 2 | 900 MB | 900 MB | 2 | | | orders | FUSED | 1 file/task | 15 | 1.38 GB | 1.38 GB | 15 | | | lineitem | SPLIT | 2 tasks/file | 60 | 2.71 GB | 1.36 GB | 120 | | | supplier | FUSED | 1 file/task | 1 | 120 MB | 120 MB | 1 | | 18 | orders | SPLIT | 2 tasks/file | 15 | 2.18 GB | 1.09 GB | 30 | | | lineitem | SPLIT | 2 tasks/file | 60 | 1.6 GB | 800.1 MB | 120 | | | customer | SPLIT | 2 tasks/file | 2 | 1.95 GB | 976.54 MB | 4 | | 21 | lineitem | FUSED | 1 file/task | 60 | 1.11 GB | 1.11 GB | 60 | | | lineitem | FUSED | 1 file/task | 60 | 1.42 GB | 1.42 GB | 60 | | | supplier | FUSED | 1 file/task | 1 | 300.41 MB | 300.41 MB | 1 | | | nation | FUSED | 1 file/task | 1 | 259 B | 259 B | 1 | | | orders | FUSED | 1 file/task | 15 | 1.3 GB | 1.3 GB | 15 | +----+----------+--------+--------------+-------+------------------------+-----------+------------+ ``` Explain output: `CUDF_POLARS__EXPLAIN__PARTITION_PLAN=1 and --explain` ``` SORT ('l_returnflag', 'l_linestatus') ('l_returnflag', 'l_linestatus', 'sum_qty', '...', 'avg_disc', 'count_order') [180] SELECT ('l_returnflag', 'l_linestatus', 'sum_qty', '...', 'avg_disc', 'count_order') [180] GROUPBY ('l_returnflag', 'l_linestatus') ('l_returnflag', 'l_linestatus', 'sum_qty', '...', 'avg_disc', '______________11') [180] HSTACK ('l_returnflag', 'l_linestatus', 'l_quantity', '...', 'l_tax', '__POLARS_CSER_0x6a4988e5b78df9ee') [180] PROJECTION ('l_returnflag', 'l_linestatus', 'l_quantity', 'l_extendedprice', 'l_discount', 'l_tax') [180] STREAMINGSCAN ('l_returnflag', 'l_linestatus', 'l_quantity', '...', 'l_tax', 'l_shipdate') [flavor=SPLIT_FILES factor=3 projected=4.35 GB] [180] ``` Authors: - Matthew Murray (https://github.com/Matt711) Approvers: - Vyas Ramasubramani (https://github.com/vyasr) - Mads R. B. Kristensen (https://github.com/madsbk) URL: NVIDIA#22945
…NVIDIA#22874) Adds some additional gtests for regex patterns to help validate optimizations in follow on PRs. This is part of splitting out some of the work for NVIDIA#21936 Authors: - David Wendt (https://github.com/davidwendt) Approvers: - Vyas Ramasubramani (https://github.com/vyasr) - Basit Ayantunde (https://github.com/lamarrr) URL: NVIDIA#22874
Closes NVIDIA#22830 This PR fuses the multi-column range window offset generation Authors: - Muhammad Haseeb (https://github.com/mhaseeb123) - Nghia Truong (https://github.com/ttnghia) Approvers: - Nghia Truong (https://github.com/ttnghia) - Bradley Dice (https://github.com/bdice) - Yunsong Wang (https://github.com/PointKernel) URL: NVIDIA#22863
This PR adds a new AI-agent skill to automatically compare the performance of branch or a PR against the `rapidsai/cudf/main` branch and produce a report Authors: - Muhammad Haseeb (https://github.com/mhaseeb123) Approvers: - Vyas Ramasubramani (https://github.com/vyasr) - Bradley Dice (https://github.com/bdice) - Yunsong Wang (https://github.com/PointKernel) URL: NVIDIA#22725
closes NVIDIA#23010 `python -m cudf.pandas --line-profile <script>` writes an *instrumented copy* of the script to a temporary file and executes it via `runpy.run_path(<temp>)`, which sets `__file__` to that temporary path. Scripts that locate sibling resources relative to `__file__` (e.g. `Path(__file__).resolve().parent.parent / "data" / "file.parquet"`) then resolve to the wrong location and fail: ``` FileNotFoundError: /data/nyc_parking_violations_2022.parquet ``` The same script runs fine **without** `--line-profile` (it is executed directly, so `__file__` is correct). ### Root cause / fix The per-line profiler needs the executed code object's filename to be the *instrumented* temp file — it reads source lines via `inspect.stack().code_context` (→ `linecache` on `co_filename`) and shifts line numbers back to the original — so the temp filename can't simply be swapped for the real one. This PR keeps the code object's filename pointed at the temp file (per-line profiling output and tracebacks are unchanged) but executes it in a `__main__` module whose `__file__` — and `sys.argv[0]` — refer to the **original** script. This matches the behavior of running without `--line-profile`, so scripts that resolve paths relative to `__file__` keep working. ### Tests Adds `test_run_cudf_pandas_line_profile_preserves_file`: runs `python -m cudf.pandas --line-profile` on a script that reads a sibling file via `__file__` and asserts it succeeds (and that `__file__` is the original script path). The function profiler (`--profile`) was never affected — it executes the original script directly. Authors: - GALI PREM SAGAR (https://github.com/galipremsagar) Approvers: - Bradley Dice (https://github.com/bdice) URL: NVIDIA#23017
|
/okay to test e06a929 |
|
/okay to test 23ffb98 |
|
/merge |
|
/okay to test f284c1f |
Makes `merge`/`join` match pandas across the dtype/index/error semantics exercised by `tests/reshape/merge/test_merge.py` under `cudf.pandas`. The changes are **not** gated on `mode.pandas_compatible` — cudf-classic now matches pandas too, and the affected cudf-classic tests are updated accordingly.
Failures in `tests/reshape/merge/test_merge.py` go from **92 → 13** under `cudf.pandas`; the full cudf-classic suite passes (80k+ tests via `pytest -n 12 --dist=worksteal`, 0 regressions), along with the dask_cudf and custreamz suites.
### What changed
- **Suffix duplicate columns** → raise `pandas.errors.MergeError` when suffixing introduces a duplicate label not already present in the inputs (previously silently dropped a column).
- **`left_index`/`right_index` must be bool** → `ValueError`.
- **Numeric-vs-string key** → `ValueError` ("You are trying to merge on ... use pd.concat") instead of silently coercing.
- **Extension-dtype keys** (nullable/pyarrow) → the retained key keeps the LEFT operand's dtype for all join types.
- **Empty-frame keys** → keep their original dtype (pandas never coerces an empty object key against an empty numeric key).
- **Categorical keys** → decategorize to the common categories dtype unless the two category sets match (up to permutation), for every join type; matching pandas.
- **Index selection** → the result keeps a frame's index only when it joined via the `*_index` flag, or the same index level is used as key on both sides (`on=`). An index level used via `left_on`/`right_on`, or a plain column merge, yields a default `RangeIndex`.
- **Unmatched rows** → coalesce the surviving key column from the opposite index (`left_on`+`right_index`), drop the index name and upcast a numpy integer index/column to `float64` (numpy has no integer NA sentinel). The upcast applies only when the merge itself *introduces* the missing values: each side's gather map is checked for unmatched entries, so a column that merely carried cudf-native nulls into the merge (which pandas cannot represent in a numpy int column) keeps its integer dtype — as does every column of a fully-matched join.
- **Numpy key dtypes** → differently-named keys keep their own dtype; same-name keys keep the LEFT dtype for inner/left joins (right/outer take the common type).
### Keeping merge semantics out of internal operations
Several cudf operations are implemented on top of `DataFrame.merge`/`join` — binop index alignment, `reindex`, `.loc` setitem value alignment, DataFrame construction from Series, `concat(axis=1)` — but pandas' *merge* dtype rules don't apply to those operations in pandas itself (e.g. `reindex(..., fill_value=0)` keeps `int64`, and misaligned binops are documented in cudf to produce nullable ints, not `float64`). Rather than gating merge behavior, the internal helpers no longer route data columns through the merge machinery:
- `_align_to_index` and `_reindex` now join **indexes only** (with a positional iota column) and gather the data columns natively afterwards, so column dtypes are untouched by alignment; `_reindex`'s own pandas dtype rules (#22914) remain the single source of reindex dtype behavior.
- `_indices_from_labels` restores the positional column's integer dtype after its label join (row positions round-trip float64 losslessly).
- Alignment call sites pre-unify unordered categorical indexes with differing category sets to a merged-categories dtype (first-appearance order), and `concat`'s combined-index computation no longer sorts categorical unions — pandas' union of categoricals decategorizes, so sorting would order lexically instead of by appearance like `pd.concat` does.
- `dask_cudf`'s `test_merging_categorical_columns` expectation is updated to the pandas-matching result (merging on categorical keys with different category sets decategorizes; verified identical to dask-with-pandas, including the dtype-mismatch `UserWarning`).
### Not addressed (inherent / out of scope, kept in the xfail list)
`test_merge_nocopy` (cudf can't share memory), `test_merge_left_empty_right_notempty` (None-vs-NaN), `test_merge_incompat_dtypes_are_ok` (cudf represents strings as `StringDtype`, not numpy `object`), the `IntervalIndex` mapped-index case, outer-merge row ordering (`intc`/`uintc`), the `Series`-with-tuple-name nlevels check, `test_merge_right_left_index` (an extra materialized key column), and a tz DST / timedelta case.
Authors:
- GALI PREM SAGAR (https://github.com/galipremsagar)
Approvers:
- Matthew Roeschke (https://github.com/mroeschke)
- Tom Augspurger (https://github.com/TomAugspurger)
URL: #23060
Aligns
DataFrame.reindexwith pandas (unconditional unless noted): infer new fill-column dtype from the fill value / homogeneous frame dtype instead of always float64, raise on duplicate target columns and invalid arg combos, treat int/float index levels as joinable, and upcast int→float when NaN-filling added rows (gated on pandas-compatible mode). Also fixesset_indexfreq preservation and empty-list column dtype.Fixes 25
frame/methods/test_reindex.pyfailures under cudf.pandas (drops their now-passing plugin entries); the rest are inherent (object-vs-str, indexis_identity, MultiIndex-level broadcast). Full cudf unit suite: 0 regressions.